discovery: default trickledelay to 1 if less is set #10463

hieblmi · 2025-12-24T09:05:37Z

This PR defaults the peer gossip trickle delay to 1 if its config value is set to <=0.

Observing the following error if --trickledelay=0:

panic: non-positive interval for NewTicker


goroutine 2212 [running]:

time.NewTicker(0x4000b47160?)

	time/tick.go:38 +0xc0

github.com/lightningnetwork/lnd/discovery.(*AuthenticatedGossiper).networkHandler(0x400086b2b0, {0x27384c8, 0x4002832b40})

	github.com/lightningnetwork/lnd/discovery/gossiper.go:1466 +0xd8

created by github.com/lightningnetwork/lnd/discovery.(*AuthenticatedGossiper).start in goroutine 2057

	github.com/lightningnetwork/lnd/discovery/gossiper.go:697 +0x15c

gemini-code-assist · 2025-12-24T09:05:49Z

Summary of Changes

Hello @hieblmi, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a crucial fix to enhance the stability of the network's gossip mechanism. It addresses a specific scenario where an invalid configuration for the TrickleDelay could lead to a program crash. By ensuring that the delay is always a positive duration, the change prevents panics and guarantees the continuous operation of the peer gossip handler, improving overall system robustness.

Highlights

Panic Prevention: This pull request resolves a critical runtime panic that occurred when the peer gossip TrickleDelay configuration was set to a non-positive value (e.g., 0s), which caused time.NewTicker to fail.
Defaulting TrickleDelay: A safeguard has been implemented in the networkHandler to check the TrickleDelay value. If it is found to be less than or equal to 0, it is now automatically defaulted to 1 nanosecond, ensuring a valid duration for time.NewTicker.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request correctly fixes a panic that occurs when trickledelay is set to a non-positive value. The change introduces a check to ensure the delay is positive before creating a ticker. My review includes a suggestion to refactor this validation logic into the constructor for better code organization.

discovery/gossiper.go

ziggie1984 · 2026-01-30T18:20:03Z

yes let's bring this change in for robustness.

lightninglabs-deploy · 2026-02-02T08:19:54Z

🟠 PR Severity: HIGH

discovery/gossiper.go | 3 files | 83 lines changed

🟠 High (1 file)

discovery/gossiper.go - Gossip protocol implementation (trickle delay configuration)

🟢 Low (1 file)

docs/release-notes/release-notes-0.21.0.md - Documentation update

📝 Test Files (1 file)

discovery/gossiper_test.go - Test coverage for trickle delay fix

Analysis

This PR modifies the gossip protocol implementation in discovery/gossiper.go to ensure the trickle delay has a minimum value of 1 when a value less than 1 is configured. This affects how quickly gossip messages are broadcast to peers.

The discovery/* package is classified as HIGH severity because it handles the gossip protocol, which is responsible for propagating channel announcements, node announcements, and channel updates across the Lightning Network. Incorrect behavior in gossip timing could affect network topology propagation.

The change is small (9 lines in production code) and appears to be a defensive fix to prevent invalid configuration values. However, given its impact on peer-to-peer message propagation timing, a knowledgeable engineer review is appropriate.

No severity bump applied: The PR touches only 1 production file with 9 lines changed (well below the thresholds of >20 files or >500 lines).

_{To override, add a severity-override-{critical,high,medium,low} label.}

lightninglabs-deploy · 2026-02-02T08:56:33Z

🟠 PR Severity: HIGH

gossiper.go change | 3 files | 82 lines changed

🟠 High (1 file)

discovery/gossiper.go - Gossip protocol configuration change (trickle delay default value)

🟢 Low (1 file)

docs/release-notes/release-notes-0.21.0.md - Documentation update

📋 Test Files (1 file - excluded from severity)

discovery/gossiper_test.go - Test coverage for trickle delay changes

Analysis

This PR modifies the gossip protocol's trickle delay configuration in discovery/gossiper.go. The change ensures that if a trickle delay value less than 1 is configured, it defaults to 1 to prevent potential issues with the gossip timing mechanism.

Why HIGH severity:

Changes core gossip protocol behavior (discovery/* is classified as HIGH)
Modifies configuration validation logic that affects network message propagation
Small, focused change (1 production file, 9 lines) - no severity bump needed

The change is defensive (setting a safe default) rather than introducing new functionality, which reduces risk. However, it still touches the gossip protocol which is a critical component for network health and message propagation.

_{To override, add a severity-override-{critical,high,medium,low} label.}

docs/release-notes/release-notes-0.21.0.md

ziggie1984 · 2026-02-02T17:22:01Z

discovery/gossiper.go

 // New creates a new AuthenticatedGossiper instance, initialized with the
 // passed configuration parameters.
 func New(cfg Config, selfKeyDesc *keychain.KeyDescriptor) *AuthenticatedGossiper {
+	if cfg.TrickleDelay <= 0 {


@gemini-code-assist does it make sense to mix the constructor of the AuthenticatedGossiper with the validation of values or should this config value be checked when we parse the config

That's a good question, @ziggie1984, and it touches on an important design consideration regarding separation of concerns.

There are valid arguments for both approaches:

Validating in the constructor (New function, as currently implemented):

Pros: This ensures that any AuthenticatedGossiper instance is always created with a valid TrickleDelay, preventing the time.NewTicker panic immediately upon object instantiation. It acts as a robust safeguard for the AuthenticatedGossiper's internal state, regardless of how the Config object was populated (e.g., directly in code, from tests, or from various config sources).

Cons: It does mix the responsibility of object construction with input validation/correction, which can sometimes be seen as a violation of the Single Responsibility Principle.

Validating during config parsing:

Pros: This would centralize all configuration validation in one place, ensuring that the Config object itself is always valid before being passed to any component. It cleanly separates the concern of config parsing and validation from the component's construction logic.

Cons: It relies on all paths that create or modify a Config object to go through this validation. If a Config struct can be manually constructed or partially updated without full validation, then invalid values could still propagate. The New function would then either have to trust the Config (risking the panic) or re-validate, leading to duplication.

Given that the primary goal here is to prevent a critical runtime panic (non-positive interval for NewTicker) that directly affects the AuthenticatedGossiper's operation, performing this validation and defaulting within the New function is a pragmatic and robust solution. It guarantees that the AuthenticatedGossiper will always receive a usable TrickleDelay value. The log.Infof message also provides valuable debugging information if an unexpected non-positive value was provided in the configuration.

While a more comprehensive, centralized config validation layer could be beneficial for the entire application, for this specific critical parameter, the current approach in the constructor provides immediate and reliable protection against a known panic.

ziggie1984

Needs a rebase, and also consider moving the check into the validate function maybe ?

hieblmi · 2026-02-02T18:26:11Z

I added commit dd29d56 to fix a resource leak in the sqldb tests.

lightninglabs-deploy · 2026-02-02T18:28:58Z

🟠 PR Severity: HIGH

gossip discovery fix | 4 files | 80 lines changed

🟠 High (1 file)

sqldb/postgres_fixture.go - SQL database fixture modification (sqldb/* category)

🟡 Medium (1 file)

config.go - Main configuration file, adds validation for trickledelay parameter

🟢 Low (2 files)

config_test.go - Test coverage for config validation
docs/release-notes/release-notes-0.21.0.md - Release notes update

Analysis

This PR fixes a panic when --trickledelay=0 is set by defaulting the value to 1 if a non-positive value is provided. The highest severity file is sqldb/postgres_fixture.go, which falls under the sqldb/* category (HIGH severity).

While the core fix in config.go is straightforward parameter validation, the inclusion of SQL database fixture changes requires review by a knowledgeable engineer familiar with database test infrastructure. The PR is small (2 non-test files, 17 substantive lines changed) and doesn't meet bump criteria.

Recommendation: Focus review on ensuring the postgres fixture changes are appropriate and that the trickledelay validation logic handles edge cases correctly.

_{To override, add a severity-override-{critical,high,medium,low} label.}

ziggie1984

LGTM (pending CI)

yyforyongyu

LGTM 🛡️

hieblmi marked this pull request as draft December 24, 2025 09:05

gemini-code-assist bot reviewed Dec 24, 2025

View reviewed changes

discovery/gossiper.go Outdated Show resolved Hide resolved

hieblmi force-pushed the fix-trickle branch from 65e0727 to f1ff0bf Compare February 2, 2026 08:18

hieblmi marked this pull request as ready for review February 2, 2026 08:22

hieblmi requested review from yyforyongyu and ziggie1984 February 2, 2026 08:22

hieblmi self-assigned this Feb 2, 2026

hieblmi added gossip config Parameters/arguments/config file related issues/PRs labels Feb 2, 2026

hieblmi force-pushed the fix-trickle branch from f1ff0bf to b98fabd Compare February 2, 2026 08:55

ziggie1984 reviewed Feb 2, 2026

View reviewed changes

docs/release-notes/release-notes-0.21.0.md Outdated Show resolved Hide resolved

ziggie1984 reviewed Feb 2, 2026

View reviewed changes

sqldb: close test db in cleanup

dd29d56

hieblmi force-pushed the fix-trickle branch from b98fabd to 8c05e0e Compare February 2, 2026 18:25

hieblmi force-pushed the fix-trickle branch from 8c05e0e to 5306be4 Compare February 2, 2026 18:27

hieblmi requested a review from ziggie1984 February 2, 2026 18:28

hieblmi added 2 commits February 2, 2026 20:47

config: default TrickleDelay to 1ms if non-positive

4ef2019

docs: update release notes for gossip trickle delay validation

22feff6

hieblmi force-pushed the fix-trickle branch from 5306be4 to 22feff6 Compare February 2, 2026 19:48

ziggie1984 approved these changes Feb 2, 2026

View reviewed changes

yyforyongyu approved these changes Feb 3, 2026

View reviewed changes

yyforyongyu merged commit 06b175a into lightningnetwork:master Feb 3, 2026
42 of 80 checks passed

hieblmi deleted the fix-trickle branch February 3, 2026 11:03

discovery: default trickledelay to 1 if less is set #10463

discovery: default trickledelay to 1 if less is set #10463

Conversation

hieblmi commented Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot commented Dec 24, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

ziggie1984 commented Jan 30, 2026

Uh oh!

lightninglabs-deploy commented Feb 2, 2026

🟠 PR Severity: HIGH

Analysis

Uh oh!

lightninglabs-deploy commented Feb 2, 2026

🟠 PR Severity: HIGH

Analysis

Uh oh!

Uh oh!

ziggie1984 Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

ziggie1984 left a comment

Choose a reason for hiding this comment

Uh oh!

hieblmi commented Feb 2, 2026

Uh oh!

lightninglabs-deploy commented Feb 2, 2026

🟠 PR Severity: HIGH

Analysis

Uh oh!

ziggie1984 left a comment

Choose a reason for hiding this comment

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hieblmi commented Dec 24, 2025 •

edited

Loading